tests/commands/admin/quote/create.test.ts (1)

16-16: queryClient mock is unused and misrepresents the real export's type.

src/commands/admin/quote/create.ts only uses db; it never touches queryClient, so this added entry isn't exercised by any test in this file. It also models queryClient as () => Promise<[]>, whereas the real export in src/database/index.ts is a postgres() client instance (ReturnType<typeof postgres>) — a callable tagged-template/SQL function, not a zero-arg function returning an array. If a future test in this file starts depending on queryClient, this shape will silently diverge from production behavior.

Consider either dropping the extra key here, or — if you're standardizing the database mock across the suite to match new consumers (e.g., health checks) — shaping it closer to the real client (e.g., a sinon stub callable as queryClient\SELECT 1`resolving to[]`) so tests fail loudly on contract drift.

♻️ Minimal cleanup (drop unused key)

-    mock.module("../../../../src/database/index.js", () => ({ db, queryClient: () => Promise.resolve([]) }));
+    mock.module("../../../../src/database/index.js", () => ({ db }));

(apply to both loadModule and loadModuleNotPermitted)

Also applies to: 31-31

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/commands/admin/quote/create.test.ts` at line 16, The test mock adds a
`queryClient` key that is unused and misrepresents the real export; update the
mock in both `mock.module(...)` calls (used by `loadModule` and
`loadModuleNotPermitted`) to either remove the `queryClient` entry entirely and
only export `db`, or replace it with a stub that mimics the real tagged-template
SQL client (e.g., a callable/stub named `queryClient` that can be invoked as a
tagged-template and resolves to an array) so the test surface matches
`src/database/index.ts`; make the change where
`mock.module("../../../../src/database/index.js", () => ({ db, queryClient: ()
=> Promise.resolve([]) }))` is defined.

src/utils/ownerGuard.ts (1)

35-48: Optional: log the not-ready path for observability.

When client.application is missing, the user gets an ephemeral reply but nothing is recorded. Given this PR’s observability focus (adding interactionId/guildId to error logs), consider emitting a warn-level log here so operators can detect if owner commands are being invoked before the client is fully ready.

Suggested addition

 export async function requireApplication(
   client: Client,
   interaction: CommandInteraction
 ): Promise<ClientApplication | null> {
   if (client.application) {
     return client.application;
   }
 
+  logger.commands.error?.(
+    "requireApplication",
+    "client.application not ready",
+    { userId: interaction.user.id, guildId: interaction.guildId }
+  );
   await interaction.reply({
     content: "Bot application is not ready. Please try again in a moment.",
     flags: MessageFlags.Ephemeral,
   });
   return null;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/ownerGuard.ts` around lines 35 - 48, When client.application is
missing in requireApplication, add a warn-level log to record the not-ready path
for observability: log a message from within requireApplication that includes
context fields such as interaction.id and interaction.guildId (and mention the
function name requireApplication in the message), using the project’s existing
logger (e.g., processLogger or the module’s logger) so operators can detect
owner commands invoked before client readiness; then keep the existing ephemeral
reply and return null.

tests/events/guildDelete.test.ts (1)

14-14: Mock shape doesn't match real queryClient export.

The real queryClient (per src/database/index.ts) is a postgres tagged-template client, not a zero-arg function returning Promise.resolve([]). It happens to be harmless here because guildDeleteEvent only touches db, but if a future change in src/events/guildDelete.ts probes the DB directly (e.g., queryClient\SELECT 1`), this mock will throw queryClient is not a function-style errors or silently diverge from production behavior. Consider stubbing it as a tag function, e.g. queryClient: Object.assign(() => Promise.resolve([]), { end: sinon.stub().resolves() })`, or omit it entirely since it isn't used by this handler.

Same applies to line 31.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/events/guildDelete.test.ts` at line 14, The test mock for queryClient
in mock.module does not match the real export shape (it's a postgres
tagged-template function), so update the mock to either omit queryClient
entirely or stub it as a tag function to mirror production (e.g., replace the
zero-arg Promise-returning function with a tag-function shaped stub assigned to
queryClient and provide an end method), ensuring the mock lives alongside the
existing db mock used by guildDeleteEvent; apply the same change to the second
occurrence referenced in the file so future uses of queryClient (e.g.,
queryClient`SELECT 1`) won't throw or diverge from runtime behavior.

tests/helpers.ts (1)

239-239: Move the discord.js import to the top and wrap the long signature.

Imports hoist in ESM so this works, but placing it mid-file is unusual and will surprise readers/linters. Line 242 also exceeds the 120-char line-length guideline.

As per coding guidelines: "Enforce maximum line length of 120 characters".

🧹 Proposed refactor

 import sinon from "sinon";
 import type { SinonStub } from "sinon";
+import { ActionRowBuilder, ButtonBuilder, ButtonStyle, EmbedBuilder } from "discord.js";
@@
-// ── Premium upsell stub (matches real premium.buildPremiumUpsell shape) ────
-import { ActionRowBuilder, ButtonBuilder, ButtonStyle, EmbedBuilder } from "discord.js";
-
-export function stubBuildPremiumUpsell(skuId?: string) {
-  return (opts: { title?: string; description?: string; fields?: { name: string; value: string; inline?: boolean }[] } = {}) => {
+// ── Premium upsell stub (matches real premium.buildPremiumUpsell shape) ────
+
+interface UpsellOpts {
+  title?: string;
+  description?: string;
+  fields?: { name: string; value: string; inline?: boolean }[];
+}
+
+export function stubBuildPremiumUpsell(skuId?: string) {
+  return (opts: UpsellOpts = {}) => {

Also applies to: 242-242

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/helpers.ts` at line 239, Move the import of ActionRowBuilder,
ButtonBuilder, ButtonStyle, and EmbedBuilder to the top of the module (before
any other code) and reformat the import statement so the specifiers are wrapped
across multiple lines to keep the line length under 120 characters (e.g. import
{ ActionRowBuilder, ButtonBuilder, ButtonStyle, EmbedBuilder } from
"discord.js"; -> import { ActionRowBuilder, ButtonBuilder, ButtonStyle,
EmbedBuilder } from "discord.js"; but with each specifier on its own line inside
the braces or split into two shorter imports) so linters and readers won’t be
surprised by a mid-file import and the line-length rule is respected.

src/utils/scheduleEvaluator.ts (1)

23-29: LGTM — strict parsing is the right call.

Regex covers the valid HH:mm space exactly, and returning false from isGuildDueForMotivation on parse failure is safer than silently coercing to defaults (which could cause unintended sends). Consider emitting a warn log once per malformed guild so operators can spot bad rows — optional.

Also applies to: 58-62

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/scheduleEvaluator.ts` around lines 23 - 29, The parse function
parseHourMinute is fine but when it returns null callers like
isGuildDueForMotivation should emit a single warn-level log per malformed guild
to help operators find bad rows; update isGuildDueForMotivation to detect a null
result from parseHourMinute and call the logger (e.g., processLogger.warn or
your app logger) including the guild identifier and the offending time string,
and ensure you only warn once per guild (track warned guild IDs in a Set scoped
to the module or service) to avoid log spam.

Dockerfile (1)

43-45: HEALTHCHECK looks good.

curl is installed in the runtime stage (line 27), ${PORT:-3000} correctly mirrors the app's bind port default, and the 20s start-period gives migrations/startup room. One small consideration: since the deep health endpoint returns 503 when DB/Redis are degraded, transient dependency blips could flap the container to unhealthy and trigger orchestrator restarts. If that becomes noisy, consider probing a lightweight liveness endpoint here and reserving /api/health for readiness.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 43 - 45, HEALTHCHECK currently probes the deep
readiness endpoint (/api/health) which can return 503 for degraded dependencies
and cause flapping; change the Docker HEALTHCHECK (symbol: HEALTHCHECK) to call
a lightweight liveness endpoint (e.g. /api/health/liveness or /api/health/live)
that only verifies the app process is responsive, keep ${PORT:-3000} as-is, and
leave the deep /api/health as the readiness probe used by orchestrator so
transient DB/Redis blips don't mark the container unhealthy.

tests/api/health.test.ts (1)

15-19: Mock shape diverges from production queryClient invocation.

src/api/routes/health.ts calls queryClient\SELECT 1`as a **tagged template** (postgres.js's query builder), whereas the stub here is a plainsinon.stub()invoked asdbStub(strings, ...values). The test still passes because sinon ignores args and resolves, but it doesn't actually verify the production call shape — if someone refactors the probe to e.g. queryClient.unsafe("SELECT 1")or to go throughdb`, these tests will continue to pass while the endpoint breaks.

Optional hardening: assert dbStub.calledOnce and/or that the first arg is an array with a raw property (tagged-template signature), to pin the contract.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/api/health.test.ts` around lines 15 - 19, The test's dbStub must mimic
the tagged-template shape used by queryClient in src/api/routes/health.ts:
replace the plain sinon.stub() with a stub that accepts (strings, ...values) as
a tagged-template handler (or use callsFake to return the resolved row when
invoked with that signature) and add assertions that dbStub was called once and
that the first argument is an array with a raw property (e.g.,
Array.isArray(args[0]) && 'raw' in args[0]) to pin the contract; keep
redisPingStub behavior but ensure you reference dbStub and the queryClient
tagged-template invocation when updating the test.

src/database/migrate.ts (1)

28-38: Consider a bounded wait on the advisory lock.

pg_advisory_lock blocks indefinitely. If a previous migration container is wedged (crash-looping, stuck on a long DDL, or a leaked connection holding the lock), every subsequent docker-entrypoint.sh startup will hang here with no diagnostic. For a rolling-deploy/replica scenario this can turn a single stuck pod into a cluster-wide startup stall.

Two common mitigations:

• Use pg_try_advisory_lock in a retry loop with a max-attempts/total-timeout and clear log output when waiting.
• Or set a session lock_timeout / statement_timeout so the blocked acquire fails fast and the entrypoint exits non-zero (letting the orchestrator surface it).

♻️ Sketch — try-lock with backoff

-try {
-  await db.execute(sql`SELECT pg_advisory_lock(${LOCK_KEY})`);
-  await migrate(db, { migrationsFolder });
-} finally {
+const LOCK_WAIT_MS = 60_000;
+const start = Date.now();
+let acquired = false;
+try {
+  while (Date.now() - start < LOCK_WAIT_MS) {
+    const [row] = await db.execute<{ locked: boolean }>(
+      sql`SELECT pg_try_advisory_lock(${LOCK_KEY}) AS locked`
+    );
+    if (row?.locked) { acquired = true; break; }
+    console.log("Waiting for migration advisory lock...");
+    await new Promise((r) => setTimeout(r, 2000));
+  }
+  if (!acquired) {
+    throw new Error(`Timed out waiting for migration advisory lock after ${LOCK_WAIT_MS}ms`);
+  }
+  await migrate(db, { migrationsFolder });
+} finally {
+  if (acquired) {
     try {
       await db.execute(sql`SELECT pg_advisory_unlock(${LOCK_KEY})`);
     } catch {
       // unlock failure is non-fatal — connection close releases it
     }
+  }
   await sqlClient.end();
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/database/migrate.ts` around lines 28 - 38, The advisory lock call using
pg_advisory_lock(${LOCK_KEY}) can block forever; change the acquire logic in
migrate.ts to use a bounded wait: replace the direct db.execute(sql`SELECT
pg_advisory_lock(${LOCK_KEY})`) with a retry loop that uses
db.execute(sql`SELECT pg_try_advisory_lock(${LOCK_KEY})`) and retries with
backoff up to a max attempts/total timeout (logging each wait), or alternatively
set a session timeout (e.g., execute SET lock_timeout/statement_timeout) before
attempting the lock so a blocked acquire fails fast; ensure the unlock/cleanup
in the finally block still runs and that sqlClient.end() is awaited as before.

src/utils/suggestionHelpers.ts (1)

13-40: Helper assumes the interaction has not been replied/deferred.

fetchPendingSuggestion unconditionally calls interaction.reply(...) on the not-found / already-reviewed paths. If a future caller ever defers the interaction first (e.g., for a slow approval flow), this will throw InteractionAlreadyReplied. Low risk given current callers (approve.ts, reject.ts) reply immediately, but worth either a JSDoc note on the precondition or a interaction.replied || interaction.deferred branch using followUp/editReply.

status.toLowerCase() is safe — status is notNull enum in the schema.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/suggestionHelpers.ts` around lines 13 - 40, fetchPendingSuggestion
currently always calls interaction.reply on the not-found and already-reviewed
branches which will throw if the interaction was previously deferred/replied;
update fetchPendingSuggestion to check interaction.replied ||
interaction.deferred and, when true, use interaction.followUp for
not-found/already-reviewed messages (or interaction.editReply if you prefer to
replace a deferred placeholder), otherwise call interaction.reply as now;
reference the fetchPendingSuggestion function and the two reply sites handling
"Suggestion with ID ..." and "This suggestion has already been ..." to apply the
conditional branching.

tests/utils/commandErrors.test.ts (1)

17-50: LGTM — covers the main happy/sad paths.

One optional gap: there's no case exercising safeErrorReply's no-op branch (handler throws after the interaction has already been replied/deferred). That path is the most likely way the wrapper can hide user-facing errors, so a small test asserting interaction.reply is not called when interaction.replied = true would round out coverage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/utils/commandErrors.test.ts` around lines 17 - 50, Add a test
exercising safeErrorReply's no-op branch by using the existing load() and
mockInteraction(), set interaction.replied = true (or mark it as
deferred/replied) before invoking mod.withCommandLogging("cmd", interaction as
never, ...) where the handler throws; then assert logger.commands.error was
called (e.g., calledOnce) and that interaction.reply was NOT called (called ===
false), so you cover the scenario where the wrapper should not attempt a reply
when interaction.replied is true.

tests/utils/env.test.ts (1)

86-116: LGTM — good coverage for the new snowflake/pool validation.

Nice to see the stricter snowflake regex covered for both single IDs and ALLOWED_USERS list entries, plus the pool-max default. One small optional addition: the PR also introduces WORKER_CONCURRENCY in the env schema, which doesn't have an analogous default-value assertion here — might be worth a one-liner for symmetry with the DATABASE_POOL_MAX test.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/utils/env.test.ts` around lines 86 - 116, Add a test mirroring the
DATABASE_POOL_MAX default check: call envSchema.safeParse(validEnv()) and assert
result.success is true, then assert result.data.WORKER_CONCURRENCY equals the
default value defined in the env schema; locate the test near the existing
"should default DATABASE_POOL_MAX to 10 when unset" case and use the same
pattern (envSchema, validEnv) so the new test verifies the WORKER_CONCURRENCY
default.

src/utils/commandErrors.ts (2)

46-71: Handler success/failure is observable only via logs — no return value.

withCommandLogging returns Promise<void> even when the wrapped handler throws (errors are swallowed after logging + reply). Callers downstream can't distinguish "handled failure" from "success," which is fine today since nobody chains off the return, but it also means a handler that returns a value can't bubble it up. Since this is a new centralized wrapper that's applied broadly, consider returning a discriminated result (or at least propagating an "ok" boolean) now to keep future callers flexible.

Non-blocking; flag and move on if you'd rather keep the surface minimal.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/commandErrors.ts` around lines 46 - 71, withCommandLogging
currently swallows handler results and always returns Promise<void>, so callers
can't tell success vs handled failure or receive a returned value; change
withCommandLogging to be generic (e.g., withCommandLogging<T>) and return a
discriminated result (e.g., { ok: true, value: T } | { ok: false, error: unknown
}) instead of void, invoke the provided handler() and on success return { ok:
true, value } (capturing any returned value), and on catch keep calling
logCommandError(commandName, interaction, err) and safeErrorReply(interaction,
errorMessage) but return { ok: false, error: err } so downstream callers can
observe outcome; update callers accordingly to handle the new return shape.

---

67-70: Secondary failure in safeErrorReply will surface as an unhandled rejection.

If interaction.reply(...) inside safeErrorReply throws (network hiccup, unknown interaction, etc.), the rejection propagates out of withCommandLogging — which is now the top-level boundary for most command handlers. Since the error-logging already happened, it's usually better to catch that reply failure and log it rather than re-raise.

♻️ Suggested fix

   } catch (err) {
     logCommandError(commandName, interaction, err);
-    await safeErrorReply(interaction, errorMessage);
+    try {
+      await safeErrorReply(interaction, errorMessage);
+    } catch (replyErr) {
+      logger.commands.error(
+        commandName,
+        interaction.user.username,
+        interaction.user.id,
+        replyErr,
+        interaction.guildId ?? undefined
+      );
+    }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/commandErrors.ts` around lines 67 - 70, The catch block in
withCommandLogging currently calls logCommandError(...) then awaits
safeErrorReply(...), but if safeErrorReply(...) rejects that secondary failure
becomes an unhandled rejection; wrap the safeErrorReply(interaction,
errorMessage) call in its own try/catch and, on error, log that secondary
failure (use logCommandError with the same commandName and interaction or a
dedicated logger) instead of rethrowing so reply failures are recorded but do
not propagate past withCommandLogging.

src/events/interactionCreate.ts (1)

34-35: Redundant commandName guard.

On CommandInteraction, commandName is a required non-empty string. The if (!commandName) {return;} guard is dead code; consider removing to keep the flow clear.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/events/interactionCreate.ts` around lines 34 - 35, Remove the redundant
guard that checks commandName in the interactionCreate handler: because
interaction is a CommandInteraction and commandName is required, delete the
lines "const { commandName } = interaction;" and "if (!commandName) {return;}"
or at minimum remove the if-check and continue using interaction.commandName
directly; update any downstream references to use interaction.commandName (or
keep destructuring without the dead check) inside the interactionCreate function
to simplify the control flow.

src/utils/quoteHelpers.ts (1)

13-20: ORDER BY random() scales poorly.

ORDER BY random() LIMIT 1 sorts the entire MotivationQuote table on each call. Fine today, but if the table grows this becomes an O(N log N) hot-path invocation per /quote and per worker send. Consider an offset-by-count approach (one count query + one OFFSET n LIMIT 1) or PostgreSQL TABLESAMPLE once the table grows. Flagging as optional given current scale.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/quoteHelpers.ts` around lines 13 - 20, getRandomMotivationQuote
currently uses ORDER BY random() which sorts the whole table; replace it with a
two-step approach inside getRandomMotivationQuote: first run a COUNT query
against motivationQuotes to get totalRows, then pick a random index n =
floor(Math.random() * totalRows) and fetch the single row with
.limit(1).offset(n) (or use PostgreSQL TABLESAMPLE when enabled) to avoid
full-table sorting; ensure you handle totalRows === 0 and return null, and
reference the function name getRandomMotivationQuote and the motivationQuotes
table when making the change.

src/worker/jobs/setActivityCore.ts (1)

22-77: Optional: tighten return shape and avoid the synthetic DB row.

Two minor cleanups:

1. setActivityCore has no declared return type and returns true / undefined / the result of _logger.warn(...) depending on the branch. Declaring Promise<void> and dropping the return true / return _logger.warn(...) values makes the contract clearer for the caller.
2. Pushing a synthetic { id: "default", ..., createdAt: new Date() } into the DB-result array mixes real DiscordActivity rows with a fake one whose id isn’t a UUID. It works today but will bite later (e.g., if anything downstream keys off id). A small union or a separate "effective list" avoids that:

♻️ Sketch

-export async function setActivityCore(client: Client, { db: _db, env: _env, logger: _logger }: SetActivityDeps) {
+export async function setActivityCore(
+  client: Client,
+  { db: _db, env: _env, logger: _logger }: SetActivityDeps,
+): Promise<void> {
   try {
     ...
     if (!client.user) {
-      return _logger.warn("Worker", "Client user is not defined, cannot set activity");
+      _logger.warn("Worker", "Client user is not defined, cannot set activity");
+      return;
     }
     ...
-    activities.push({
-      id: "default",
-      activity: defaultActivity,
-      type: defaultActivityType,
-      url: defaultActivityUrl ? defaultActivityUrl : null,
-      createdAt: new Date(),
-    });
-
-    const randomIndex = Math.floor(Math.random() * activities.length);
-    const activity = activities[randomIndex];
+    const pool: Array<{ activity: string; type: string; url: string | null }> = [
+      ...activities,
+      { activity: defaultActivity, type: defaultActivityType, url: defaultActivityUrl ?? null },
+    ];
+    const activity = pool[Math.floor(Math.random() * pool.length)];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/worker/jobs/setActivityCore.ts` around lines 22 - 77, setActivityCore
currently returns inconsistent shapes and mutates the DB result by pushing a
synthetic row; change the function signature to return Promise<void>, stop
returning values from branches (call _logger.warn/_logger.success and then
return), and avoid inserting a fake DB row into activities — instead build a
separate effectiveActivities array (e.g., const effective = [...activities,
defaultActivityObj]) or pick the default as a fallback variable, then choose a
random element from that effective list; keep usage of getActivityType and
client.user.setActivity as-is and ensure the catch still logs via _logger.error.

src/database/schema.ts (1)

6-52: Enum + index additions LGTM.

suggestionStatusEnum properly types the status column, and the two btree indexes target known hot paths (motivationChannelId for the guild lookup in the send-motivation worker, status for the Pending-suggestion scans used by fetchPendingSuggestion and the stats queries).

One tiny optional refinement: you can derive SuggestionStatus from the enum to keep the values in a single place:

♻️ Optional

-export type SuggestionStatus = "Pending" | "Approved" | "Rejected";
+export type SuggestionStatus = (typeof suggestionStatusEnum.enumValues)[number];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/database/schema.ts` around lines 6 - 52, Add a derived TypeScript type
for the enum so the allowed status values are sourced from suggestionStatusEnum
rather than duplicated strings: export a type alias named SuggestionStatus that
is inferred from suggestionStatusEnum (so any references to SuggestionStatus in
code use this alias), leaving the existing suggestionStatusEnum,
suggestionQuotes.status column, and index unchanged.

src/worker/index.ts (1)

14-34: Consider migrating to upsertJobScheduler for idempotent scheduler management.

BullMQ v5.16.0+ recommends Queue.upsertJobScheduler() over the remove-then-re-add pattern. It is idempotent by scheduler ID, so you don't enumerate and delete prior repeatables, and it avoids resetting the next-run anchor on every startup (your current code defers the next run by one full interval each deploy — a 10-minute activity interval means the first rotation after a deploy is ~10 minutes out).

♻️ Suggested migration

await queue.upsertJobScheduler(
  name,
  { every: intervalMs },
  { name, data, opts: { removeOnComplete: { count: 50 }, removeOnFail: { count: 100 } } }
);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/worker/index.ts` around lines 14 - 34, The ensureRepeatable function
currently removes all repeatables then re-adds a job which is non-idempotent and
resets the next-run anchor; replace that logic by calling
Queue.upsertJobScheduler instead of enumerating/removing and queue.add. Use the
job scheduler id (reuse the existing name) and pass the schedule ({ every:
intervalMs }) and the job payload/opts (data and removeOnComplete/removeOnFail
counts) so the scheduler is updated idempotently; update the ensureRepeatable
implementation to call queue.upsertJobScheduler(name, { every: intervalMs }, {
name, data, opts: { removeOnComplete: { count: 50 }, removeOnFail: { count: 100
} } }).

src/app.ts (1)

94-99: Prefer redis.quit() for graceful shutdown.

disconnect() tears the socket down immediately, potentially dropping in-flight commands (including BullMQ writes that may be mid-flush). quit() sends a QUIT to the server, waits for the acknowledgement, and then closes — which is what a graceful-shutdown path typically wants. This pattern is already used for PostgreSQL in the preceding lines. You already tolerate failures with a try/catch, so swapping in quit() is low-risk.

♻️ Suggested change

   try {
-    redis.disconnect();
+    await redis.quit();
     logger.info("App", "Redis disconnected");
   } catch (err) {
     logger.warn("App", "Error disconnecting Redis", { error: err });
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/app.ts` around lines 94 - 99, Replace the immediate socket teardown with
a graceful Redis shutdown by calling redis.quit() instead of redis.disconnect()
in the teardown block (the try/catch that currently calls redis.disconnect());
keep the existing logger messages and error handling but update the success log
text if desired (e.g., "Redis quit" or keep "Redis disconnected") and ensure the
catch continues to log the caught error via logger.warn with the error object.

tests/worker/sendMotivation.test.ts (2)

77-94: Optional: add an ordering assertion for claim-before-send.

The headline contract of this PR is that db.update (the atomic claim) must run before channel.send for each guild. A call-ordering assertion would guard against a future refactor accidentally reintroducing send-before-claim:

Suggested addition

     expect(db.update.calledOnce).toBe(true);
     expect(sendStub.calledOnce).toBe(true);
+    expect(sendStub.calledAfter(db.update)).toBe(true);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/worker/sendMotivation.test.ts` around lines 77 - 94, Add an ordering
assertion to ensure the atomic claim runs before the send: after invoking
sendMotivation and before final expectations, assert that the db.update call
(the claim) occurred before the channel.send call (sendStub) — e.g., use
sinon.assert.callOrder(db.update, sendStub) or
expect(db.update.calledBefore(sendStub)). This ties the test to the unique
symbols sendMotivation, db.update and sendStub (channel.send) so future
refactors can't reorder claim and send.

---

130-149: Consider tightening the per-guild isolation assertion.

This test stubs db.update to always return a winning claim, and stubs send to fail then succeed. It only asserts logger.error.called, which would also pass if both calls failed. A stronger assertion would verify exactly one failure and one success were recorded (e.g. logger.error.calledOnce and some indication of the sent=1 count), which is the actual behavior under test.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/worker/sendMotivation.test.ts` around lines 130 - 149, The test should
assert per-guild isolation more strictly: after calling sendMotivation, replace
the loose expect(logger.error.called).toBe(true) with assertions that exactly
one send failure was logged (e.g. logger.error.calledOnce) and that the success
path recorded a single sent count (e.g. logger.info or whichever logging call
records "sent=1" was called with the expected sent=1 indicator); also ensure
db.update/send stubs remain as-is to exercise the first-fail/second-succeed
scenario. Target symbols: sendMotivation, logger, db.update, and the
channel.send stub to locate the assertions to change.

drizzle/0000_loud_mother_askani.sql (1)

1-48: Migration looks consistent with src/database/schema.ts.

Enums, tables, and the two btree indexes (guild_motivation_channel_idx, suggestion_status_idx) match the schema. The claim‑update path in sendMotivation.ts filters by guilds.id (primary key), so no additional index is needed to support it.

Note: updatedAt is only defaulted on insert here; row updates rely on Drizzle's $onUpdate at the application layer rather than a DB trigger. That's consistent with the rest of the project but something to keep in mind if raw SQL writes are ever introduced.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@drizzle/0000_loud_mother_askani.sql` around lines 1 - 48, Migration SQL
matches src/database/schema.ts: enums
DiscordActivityType/MotivationFrequency/SuggestionStatus, tables
DiscordActivity/Guild/MotivationQuote/SuggestionQuote, and indexes
guild_motivation_channel_idx/suggestion_status_idx are correct; no functional
changes required—approve as-is and keep in mind updatedAt is application-managed
via Drizzle $onUpdate (no DB trigger) if raw SQL writes are added later.

src/utils/envSchema.ts (1)

58-64: ALLOWED_USERS validates a trimmed form but doesn't normalize the parsed value.

The refine validates by trimming entries (e.g., "123…, 456…" with space passes), but returns the raw string as-is — consumers must manually .split(",").map(s => s.trim()) or risk ID mismatches.

The sole consumer, src/utils/permissions.ts, already does this (line 10: env.ALLOWED_USERS?.trim()), and tests explicitly verify whitespace handling works. However, requiring every consumer to repeat this logic is fragile.

Consider using .transform() to normalize to string[] so validation and parsing align:

Proposed refactor

   ALLOWED_USERS: z
     .string()
     .optional()
-    .refine(
-      (v) => !v || SNOWFLAKE_LIST.test(v.split(",").map((s) => s.trim()).join(",")),
-      "ALLOWED_USERS must be a comma-separated list of Discord snowflakes"
-    ),
+    .transform((v) => (v ? v.split(",").map((s) => s.trim()).filter(Boolean) : []))
+    .refine(
+      (arr) => arr.every((id) => SNOWFLAKE.test(id)),
+      "ALLOWED_USERS must be a comma-separated list of Discord snowflakes"
+    ),

This is a breaking type change (string → string[]), so src/utils/permissions.ts and call sites need updating.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/envSchema.ts` around lines 58 - 64, ALLOWED_USERS currently
validates trimmed values but leaves the raw string intact; change its schema to
use .transform() to normalize and output a string[] (split by "," and trim each
entry) so validation and parsed value align, update the consumer permissions
logic in src/utils/permissions.ts (which references env.ALLOWED_USERS and
currently calls .trim()) to accept the new string[] shape and update any call
sites/tests accordingly to handle arrays instead of a comma-separated string.

src/worker/jobs/sendMotivation.ts (1)

21-31: Use startOf("isoWeek") instead of startOf("week") for deterministic week boundaries.

startOf("week") defaults to the locale's week start (Sunday in 'en' locale), making the delivery period non-deterministic. For consistent Monday-start ISO 8601 boundaries, import and extend the isoWeek plugin:

Required changes

 import dayjs from "dayjs";
 import utc from "dayjs/plugin/utc.js";
 import timezone from "dayjs/plugin/timezone.js";
+import isoWeek from "dayjs/plugin/isoWeek.js";
 import type { Client } from "discord.js";

 dayjs.extend(utc);
 dayjs.extend(timezone);
+dayjs.extend(isoWeek);

     case "Weekly":
-      return now.startOf("week").utc().toDate();
+      return now.startOf("isoWeek").utc().toDate();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/worker/jobs/sendMotivation.ts` around lines 21 - 31, The weekly boundary
is non-deterministic because periodStart uses dayjs().startOf("week"); update
the file to import the isoWeek plugin from dayjs (e.g., dayjs/plugin/isoWeek)
and call dayjs.extend(isoWeek) once, then change the Weekly branch in
periodStart(guild: Guild) to use startOf("isoWeek") so weeks use ISO
Monday-based boundaries (leave Daily and Monthly branches unchanged and keep
referencing guild.motivationFrequency).